Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[16.0][FIX] accounting menu not translatable #4298

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from

Conversation

primes2h
Copy link
Contributor

Risolve #2885 per la v.16.0

Copy link
Contributor

@andreampiovesana andreampiovesana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@MaurizioPellegrinet MaurizioPellegrinet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@francesco-ooops
Copy link
Contributor

/ocabot rebase

@OCA-git-bot
Copy link
Contributor

Congratulations, PR rebased to 16.0.

@OCA-git-bot OCA-git-bot force-pushed the 16.0-fix-accounting-menu branch from bb16b31 to a33f203 Compare July 30, 2024 15:22
@francesco-ooops francesco-ooops linked an issue Jul 30, 2024 that may be closed by this pull request
3 tasks
@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

Copy link
Contributor

@andreampiovesana andreampiovesana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Merge?

Copy link
Contributor

@SirAionTech SirAionTech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grazie della PR!
Come scrissi in #2675 (review):

Secondo me il problema è più generico di account vs l10n_it_account vs account_accountant quindi si potrebbe aprire una issue.

e andrebbe quindi risolto direttamente in Odoo.

Rispetto ad allora però ho trovato dei possibili problemi con questo approccio, vedi il commento qui sotto.

Se vogliamo continuare ad usare dei work-around, si potrebbe usare un approccio più dinamico, tipo

# Move the RiBa menu if any of
# its siblings (menu having same parent before write)
# is moved (parent changes).
# This happens when account_accountant (enterprise)
# is installed or uninstalled.
root_riba_menu = self.env.ref("l10n_it_riba.menu_riba")
parent_riba_menu = root_riba_menu.parent_id
if old_parent == parent_riba_menu and new_parent_id != old_parent.id:
root_riba_menu.parent_id = new_parent_id
.

l10n_it_account/views/account_menuitem.xml Outdated Show resolved Hide resolved
@kamzata
Copy link

kamzata commented Oct 20, 2024

Please, can anyone merge this? LGTM?

@TheMule71 TheMule71 mentioned this pull request Nov 8, 2024
33 tasks
@primes2h primes2h force-pushed the 16.0-fix-accounting-menu branch from a33f203 to 79bc62d Compare November 12, 2024 17:42
@primes2h
Copy link
Contributor Author

primes2h commented Nov 12, 2024

Grazie della PR! Come scrissi in #2675 (review):

Secondo me il problema è più generico di account vs l10n_it_account vs account_accountant quindi si potrebbe aprire una issue.

e andrebbe quindi risolto direttamente in Odoo.

Ho approfondito la questione e ho scoperto che tecnicamente questo problema non è un vero e proprio bug.
Da quello che ho capito, il problema di fondo è che in Odoo non è ancora stato implementato un meccanismo di inherit per i menu e finché non verrà fatto non sarà possibile risolvere correttamente la cosa.

Questo comporta che quando ad esempio vado a modificare una voce di menù in questo modo, quello che succede è che per assurdo viene modificata la stringa sorgente del modulo account.
Infatti esportando il file it.po di account dentro troviamo

msgid "Accounting"
msgstr "Fatturazione"

Questo spiega perfettamente il comportamento attuale di Odoo quando viene installato il modulo l10n_it_account. In inglese compare il menù Accounting e in italiano compare Fatturazione.

Rispetto ad allora però ho trovato dei possibili problemi con questo approccio, vedi il commento qui sotto.

Concordo con te che questo approccio apre le porte a diversi potenziali (e reali) problemi.

Se vogliamo continuare ad usare dei work-around, si potrebbe usare un approccio più dinamico, tipo

# Move the RiBa menu if any of
# its siblings (menu having same parent before write)
# is moved (parent changes).
# This happens when account_accountant (enterprise)
# is installed or uninstalled.
root_riba_menu = self.env.ref("l10n_it_riba.menu_riba")
parent_riba_menu = root_riba_menu.parent_id
if old_parent == parent_riba_menu and new_parent_id != old_parent.id:
root_riba_menu.parent_id = new_parent_id

.

Un approccio del genere applicato a questo caso porterebbe a mio parere a complicare ancora di più le cose con il forte rischio di effetti collaterali futuri non prevedibili (es. interazioni con metodi per far funzionare correttamente i menù con il modulo account_accountant, come ad es. il modulo riba o altri in OCA).

Per risolvere alla radice la problematica propongo un nuovo approccio, è un workaround che va a modificare direttamente la stringa tradotta in account.
Agli effetti pratici il risultato che si ottiene equivale a quello che fa il sistema con la stringa sorgente.
In questo modo il menù resta quello originale e funziona correttamente con qualsiasi modulo extra che va ad aggiungere sottomenù.

Cosa ne pensi?

Copy link
Contributor

@SirAionTech SirAionTech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per risolvere alla radice la problematica propongo un nuovo approccio, è un workaround che va a modificare direttamente la stringa tradotta in account.
Agli effetti pratici il risultato che si ottiene equivale a quello che fa il sistema con la stringa sorgente.
In questo modo il menù resta quello originale e funziona correttamente con qualsiasi modulo extra che va ad aggiungere sottomenù.

Cosa ne pensi?

Molto meglio grazie, e ho anche imparato qualche trucchetto su come funzionano le traduzioni, tipo _get_stored_translations e update_raw 😉.

Più in generale, dici che è fattibile aggiungere un test?

l10n_it_account/models/ir_ui_menu.py Outdated Show resolved Hide resolved
l10n_it_account/models/base_language_install.py Outdated Show resolved Hide resolved
l10n_it_account/models/ir_ui_menu.py Outdated Show resolved Hide resolved
l10n_it_account/models/base_language_install.py Outdated Show resolved Hide resolved
@primes2h primes2h force-pushed the 16.0-fix-accounting-menu branch 2 times, most recently from 1a7078f to cdb446f Compare November 25, 2024 16:34
@primes2h
Copy link
Contributor Author

Per risolvere alla radice la problematica propongo un nuovo approccio, è un workaround che va a modificare direttamente la stringa tradotta in account.
Agli effetti pratici il risultato che si ottiene equivale a quello che fa il sistema con la stringa sorgente.
In questo modo il menù resta quello originale e funziona correttamente con qualsiasi modulo extra che va ad aggiungere sottomenù.
Cosa ne pensi?

Molto meglio grazie, e ho anche imparato qualche trucchetto su come funzionano le traduzioni, tipo _get_stored_translations e update_raw 😉.

Più in generale, dici che è fattibile aggiungere un test?

Adesso non riesco, se possibile sarebbe meglio procedere senza. Si può sempre aggiungere successivamente con un'altra PR.

@primes2h primes2h force-pushed the 16.0-fix-accounting-menu branch from cdb446f to 0f30c33 Compare November 26, 2024 08:23
@primes2h primes2h requested a review from SirAionTech November 26, 2024 08:44
@primes2h primes2h force-pushed the 16.0-fix-accounting-menu branch from 0f30c33 to 463f2e4 Compare November 26, 2024 09:02
@primes2h
Copy link
Contributor Author

C'erano errori nei test indipendenti da questa PR, ho provato a far ripartire i controlli.

@primes2h
Copy link
Contributor Author

I test falliscono a causa di OCA/account-financial-reporting#1243 (vedi #4461).

Copy link

@MaurizioPellegrinet MaurizioPellegrinet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test funzionale: OK

Copy link
Member

@mymage mymage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Nel runboat ho provato a mettere Francese e ovviamente mostra la stringa errata.
Penso sarebbe carino in termini community segnalare, non so dove e come, ai cugini transalpini e agli amici iberici questa modifica.

@SirAionTech
Copy link
Contributor

/ocabot rebase

@OCA-git-bot
Copy link
Contributor

Congratulations, PR rebased to 16.0.

@OCA-git-bot OCA-git-bot force-pushed the 16.0-fix-accounting-menu branch from d542384 to 4643cbf Compare December 13, 2024 17:00
Copy link
Contributor

@SirAionTech SirAionTech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ho fatto qualche prova e ora funziona come dovrebbero fare le traduzioni, grazie!
Ho anche fatto revisione del codice e per me è ok.
Oltre ai commenti qui sotto, rimane solo l'onnipresente

è fattibile aggiungere un test?

Ma secondo me non c'è più nulla di bloccante per il merge.

Sono poi d'accordo con #4298 (review):

sarebbe carino in termini community segnalare, non so dove e come, ai cugini transalpini e agli amici iberici questa modifica.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Le modifiche a questo file si possono rimuovere? Sono solo di formattazione e non servono allo scopo del commit.
Se le vuoi mantenere, sarebbero da spostare in un commit di refactoring.


translations["it_IT"] = "Contabilità"
self.env.cache.update_raw(menu_finance, field_name, [translations], dirty=True)
self.modified(["name"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.modified(["name"])

Questo serviva quando self era un menu, giusto? Se è così ora si dovrebbe poter rimuovere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Menù "Accounting" non traducibile.
8 participants